One question is whether these should be called max-memory-per-node or max-total-memory-per-node. This would be more consistent with the node configuration property names, but less accurate?

The commit subject looks too long, here is the 7 rules: https://chris.beams.io/posts/git-commit/

- Separate subject from body with a blank line
- Limit the subject line to 50 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line
- Wrap the body at 72 characters
- Use the body to explain what and why vs. how

Note sometimes it's too difficult to limit the subject line to 50 characters, but we still want to keep as short as possible (e.g. 60 characters )

What about Add task memory related session properties and explain what added in commit message ? :)

I would call them max_memory_per_node for consistency. In general our session properties are snake case versions of the config properties. Naming them differently makes it hard to reason about what configuration properties are being overridden.

Please make sure the tests pass

@rschlussel made the requested changes, tests are passing.

Currently, we do consider max-query-memory and max-query-total-memory in ClusterMemoryManager to kill a query when it uses more memory. Do we want to do the same with this new session properties as well?

Per node memory limits are enforced at the node as the memory is claimed. Why does ClusterMemoryManager need to be involved?

Also you are quoting max-query-memory and max-query-total-memory, these are pre-existing session properties that already work and are consulted by ClusterMemoryManager. Did you mean the per-node variants.

Yes i meant the new per node memory properties. Didn't know that we already have some checks at node level. If it's already been done then it's fine.

A couple questions

1. Can we remove the direct usage of the configuration property from SqlTaskManager?
2. Should we throw an exception if someone tries to configure the session property greater than the config property so we don't run into problems with users configuring this inappropriately. (setting the maxMemoryPeroNode too high would also lead to a misconfigured reserved pool if the reserved pool is enabled)

1. Can we remove the direct usage of the configuration property from SqlTaskManager?

Where would be the appropriate place to query the session?

2. Should we throw an exception if someone tries to configure the session property greater than the config property so we don't run into problems with users configuring this inappropriately. (setting the maxMemoryPeroNode too high would also lead to a misconfigured reserved pool if the reserved pool is enabled)

Absolutely! Good catch.

1. Can we remove the direct usage of the configuration property from SqlTaskManager?

Where would be the appropriate place to query the session?

I mean that it looks like we set maxQueryUserMemoryPerNode in the query context directly from the config here:

We shouldn't need that anymore since when we need that information we get it from the session property

Also, the nodeMemoryConfig that's getting passed in to createQueryContext isn't ever used.

I mean that it looks like we set maxQueryUserMemoryPerNode in the query context directly from the config here:

presto/presto-main/src/main/java/com/facebook/presto/execution/SqlTaskManager.java

Lines 155 to 160 in 1ec8138

	DataSize maxQueryUserMemoryPerNode = nodeMemoryConfig.getMaxQueryMemoryPerNode();
	DataSize maxQueryTotalMemoryPerNode = nodeMemoryConfig.getMaxQueryTotalMemoryPerNode();
	DataSize maxQuerySpillPerNode = nodeSpillConfig.getQueryMaxSpillPerNode();
	queryContexts = CacheBuilder.newBuilder().weakValues().build(CacheLoader.from(
	queryId -> createQueryContext(queryId, localMemoryManager, nodeMemoryConfig, localSpillManager, gcMonitor, maxQueryUserMemoryPerNode, maxQueryTotalMemoryPerNode, maxQuerySpillPerNode)));

We shouldn't need that anymore since when we need that information we get it from the session property

So the problem with removing the initial values from the query context construction is that we can technically construct the query context using only a task ID (no session). It seems safer to have an initial value for those based on the configuration rather then 0 or something hard coded.

(We talked about this in person just copying it here for posterity).

I removed NodeMemoryConfig from createQueryContext.